-
Notifications
You must be signed in to change notification settings - Fork 207
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for Cart and Checkout Blocks #1467
Conversation
woocommerce-gateway-stripe.php
Outdated
// priority is important here because this ensures this integration is | ||
// registered before the WooCommerce Blocks built-in Stripe registration. | ||
// Blocks code has a check in place to only register if 'stripe' is not | ||
// already registered. | ||
add_action( | ||
'woocommerce_blocks_payment_method_type_registration', | ||
function( Automattic\WooCommerce\Blocks\Payments\PaymentMethodRegistry $payment_method_registry ) { | ||
$container = Automattic\WooCommerce\Blocks\Package::container(); | ||
// registers as shared instance. | ||
$container->register( | ||
WC_Stripe_Blocks_Support::class, | ||
function() { | ||
return new WC_Stripe_Blocks_Support; | ||
} | ||
); | ||
$payment_method_registry->register( | ||
$container->get( WC_Stripe_Blocks_Support::class ) | ||
); | ||
}, | ||
5 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks to this code, by registering this integration earlier than the Blocks integration (which uses the same hook at priority 10), this basically ensures that the Stripe extension is what gets used for the integration rather than what is embedded in WooCommerce Blocks.
The latest commit implements a new configuration property available from the blocks registration API for registering the client side handling of saved tokens. The code implementation should be fairly straightforward and you'll see I was able to re-use the existing The new configuration property does require the code added in this pr of the blocks feature plugin which we plan on including in the pending This means that while the integration should work with any version of Blocks including the Payment Method integration API (as already checked in this pr), the bug related to payment intents on saved tokens will only be fixed for users with Blocks version 4.7.0+. So this could impact considerations around how you roll this out and you may want to consider only having the integration surface for users of Blocks 4.7.0+ (which is not hard coded in this PR). Also, as noted in our Slack convo, there was also mention of 3DS cards used through express payments not being covered by payment intents in this PR. I don't think the fix for that will require a change in the Blocks integration code, you should be able to utilize the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nerrad I left a couple of comments of things that I noticed while using this PR as a reference for other integrations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll be splitting the review up a bit, just for convenience. This review is for the PHP part of the code — the smallest subset of things that changed.
I see a couple of things we need to change, but mostly just some discussion points so we can decide on the best way to maintain the code going forward.
We also need to rebase this on trunk
so we can figure out how to get tests working, and so the phpcs
config will finally work :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple of nitpicky things in the Credit Card section of the blocks client, but otherwise looking good 👏
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I notice we're using a lot of ||
fallthroughs throughout the whole JS codebase, and I think it'd be better if we use the ??
null coalescing operator more often. It's stricter, and as such will be of more help when checking for fallthrough conditions.
Some minor comments, and we still need to apply the 3DS fix that's already been applied to the CC form, but aside from that this looks great 👏
Thanks for the review @reykjalin - just to be clear, I consider this code to be handed off to you folks for any additional work/changes that you want to make, so I'm operating under the assumption that I can provide some feedback responses on review comments (especially where requested) for additional context, but it's ultimately up to you and your team what direction you take on changes. |
Sorry for any confusion here @nerrad! I'm operating under the same assumption, and reviewed the code as such 🙂 That's why I pinged you in just some comments, not all of them. Feel free to unsubscribe from the PR; I'll ping you here (or in Slack if it's urgent) if we need your input on something 🙂 Thank you for the work you've been putting into this up until this point! Excellent work 🥇 👏 🙇 |
0c182f1
to
55a6339
Compare
@reykjalin I didn't read all comments here, so forgive me if this was already discussed, but I was doing some testing in the Stripe plugin for the Cart block & Payment Request compatibility in WCPay, and I noticed the functions used to calculate shipping options in the Payment Request button are different with this integration. The functions in |
@v18 I found a way to — at least — partially reproduce the issue in Brave (Chrome):
Once you choose an address, the fields should appear like the screenshot. You don't even have to complete the payment. |
@v18 I found the problem. I forgot to remove a Fixed in 355de59. |
@reykjalin - niice. |
PRB = Payment Request Buttons (Google Pay/Apple Pay/etc.)
Specifically prevent the 3ds handler from blocking the `onCheckoutAfterProcessingWithSuccess` event emitter.
…for product_page shortcode (#1530) * Add compatibility for non-default cart/checkout block page * Add support for product_page shortcode
…kouts (#1545) We now control the PRBs entirely through AJAX APIs. Translation strings have also been updated.
6d35a38
to
b60b327
Compare
@ricardo said the following in #1545 (comment):
I just realized that part of this check is necessary. When a browser doesn't support payment requests, the browser can still load the Stripe JS object. Because of this we need to create a fake An example of the issue I'm talking about:
This is very confusing UX, so we need to fix that. This is what the checkout it supposed to look like when the Stripe JS object loads, but payment requests aren't supported:
EDIT: I fixed this in ddbdbbc by applying an approach we removed in #1545. |
Changes proposed in this Pull Request:
Issue: Link to the GitHub issue this PR addresses (if appropriate).
Fixes woocommerce/woocommerce-blocks#3172
Fixes #1455
Fixes #1503
Fixes #1518
Description:
This pull request migrates the integration code for Stripe that is currently temporarily embedded in the WooCommerce Blocks plugin into this extension which is where it should live. The integration was done in the Blocks repo initially to help with building and validating the payment method integration API interface.
While I originally thought about splitting this work into two PRs (one for infrastructure/build tools and the other for the actual integration) I decided that it's easier to test with one PR. To help with reviewing, the work is split into three commits: one for the infrastructure/build tool changes, one for the client (frontend JS) code, and one for the PHP side code.
Infrastructure/Build Tool Changes
The following is a summary of what was implemented here:
client/blocks/index.js
as the entrypoint.@woocommerce/eslint-plugin
along with eslint and prettier configuration for the new JS files. Older JS files inassets/
are ignored for now.@woocommerce/scripts
for various things including the Webpack scripts.package.json
for building includingwebpack
specific build and start commands along with adding the webpack build to thebuild
command for releases.wp-scripts
babel configuration).Testing instructions
Testing instructions: How should this be tested and how can a reviewer test the end-user functionality? Are there known issues that you plan to address in a future PR? Are there any side effects that readers should be aware of?
Testing Block integration:
You'll need to have the WooCommerce Blocks installed and active for the tests. You can use the latest released version in WordPress.org and it does not have to be downloaded from the repository.
Note: in testing, you'll want to verify that the integration from this PR is being used rather than what is included in WooCommerce Blocks. You can do this by checking which source files are loaded by the browser.
For the most part we test the payment flows in the Cart and Checkout block so if there aren't any console errors and things are loading okay in the above smoke tests then the supported payment flows should work the same here. If you encounter bugs here, then it's highly likely the bugs exist in the embedded integration in the Blocks repo as well (so we'll want to verify that).
grunt
.